Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Add RDataSource for TTree #17895

Merged
merged 3 commits into from
Mar 27, 2025

Conversation

vepadulano
Copy link
Member

All tests are working on my machine with these changes, so I want to start testing on all platforms. Changes will be later split in multiple commits/PRs.

@vepadulano
Copy link
Member Author

vepadulano commented Mar 5, 2025

Sibling roottest PR at root-project/roottest#1270 roottest tests have already been taken care of

Copy link

github-actions bot commented Mar 6, 2025

Test Results

    17 files      17 suites   4d 12h 35m 47s ⏱️
 2 737 tests  2 730 ✅   0 💤 7 ❌
45 068 runs  44 907 ✅ 154 💤 7 ❌

For more details on these failures, see this check.

Results for commit 409d22f.

♻️ This comment has been updated with latest results.

@vepadulano vepadulano marked this pull request as ready for review March 19, 2025 08:19
@vepadulano vepadulano requested review from dpiparo and hageboeck March 19, 2025 08:19
Copy link
Member

@hageboeck hageboeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a few questions and comments.
One general thing I noticed is that close to none of the functions have documentation. It can probably be retrofitted, but it also stopped me from reviewing what's being done.

@vepadulano vepadulano force-pushed the rdf-ttree-datasource branch 2 times, most recently from 5995528 to 186dbf8 Compare March 20, 2025 14:45
A new RDataSource derived class is implemented for TTree data processing. The
class follows almost entirely the same approach for processing of other data
sources, with the exception of the MT case which still needs to be partially
handled externally by TTreeProcessorMT. Nonetheless, the class and its API can
be used throughout the RDF codebase to centralise all TTree-related usage and
processing. While implementing the class, a few missing features or limitations
of RDataSource were found which needed API extensions. For the moment, all the
API extensions of RDataSource are private and it will be later decided whether
to make them public or not.
The rest of the RDF codebase is adapted to use the new TTree data source. All
the previous usage of raw TTree classes or API is now delegated to the
RDataSource. With this commit, there are no more ways to create an RDataFrame
processing a TTree without an RDataSource.
Shows the usual errors on Windows that a file cannot be removed, even though nothing is using it.
@vepadulano vepadulano force-pushed the rdf-ttree-datasource branch from 186dbf8 to 409d22f Compare March 27, 2025 12:07
Copy link
Contributor

@martamaja10 martamaja10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we discussed today, the changes look all good to me. A lot of work went into this, thanks a lot Vincenzo!

@vepadulano vepadulano merged commit d5d7921 into root-project:master Mar 27, 2025
19 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants